Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix : Empty last List Element should not be displayed #66344

Closed
wants to merge 16 commits into from
Closed

Fix : Empty last List Element should not be displayed #66344

wants to merge 16 commits into from

Conversation

nani-samireddy
Copy link

@nani-samireddy nani-samireddy commented Oct 22, 2024 •
edited
Loading

What?

Closes #65876

As mentioned in the above issue. When we create a list and if we leave the last item empty. It renders a empty list item on the front-end.

Why?

When the user adds a list item then we are saving it as list item block. But if the list-item has no content then we are saving empty list-item. Resulting in a empty list-item element

How?

  • Added a check to only save the list-item only if it has content

Testing Instructions

  • Open the Gutenberg Editor.
  • Add the list block
  • Add two or three items and for the last item keep it empty
  • On the front-end it should not display the last list-item ( empty one)

Testing Instructions for Keyboard

Screenshots or screencast

before

Screenshot 2024-10-22 at 11 21 09 PM Screenshot 2024-10-22 at 11 20 59 PM

After

Screenshot 2024-10-22 at 11 17 20 PM Screenshot 2024-10-22 at 11 17 39 PM

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 22, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: nani-samireddy <[email protected]>
Co-authored-by: up1512001 <[email protected]>
Co-authored-by: Mamaduka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 22, 2024
@github-actions GitHub Actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @nani-samireddy! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@nani-samireddy nani-samireddy changed the title Add check for empty list item Fix : Empty last List Element should not be displayed Oct 22, 2024
@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Block] List Affects the List Block labels Oct 23, 2024
@Mamaduka Mamaduka requested a review from ellatrix October 23, 2024 03:34
@nani-samireddy nani-samireddy mentioned this pull request Dec 13, 2024
Open
up1512001
up1512001 approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@up1512001 up1512001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Mamaduka
Mamaduka requested changes Dec 13, 2024
View reviewed changes
Copy link
Member

@Mamaduka Mamaduka left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure about and looking for the feedback from @ellatrix!

@up1512001, you can also see that tests are failing, with usually means that PR isn't ready to be merged.

@up1512001
Copy link
Member

up1512001 commented Dec 13, 2024 •
edited
Loading

I checked the test failure issue and noticed that its due to this file test/integration/fixtures/documents/mixed-content-out.html.
seems like we need to change this

<ul class="wp-block-list"><!-- wp:list-item -->
<li></li>
<!-- /wp:list-item -->

with this

<ul class="wp-block-list"><!-- wp:list-item /-->

as on frontend we are not putting empty li with early return.
@Mamaduka let me know if I am right 🙇

@Mamaduka
Copy link
Member

@up1512001, @nani-samireddy, I also see failures in e2e tests.

@up1512001
Copy link
Member

@Mamaduka one thing I notice during debugging e2e that to run it in watch mode we need to change package.json script from this "test:e2e:watch": "npm run test:e2e -- --watch", to this "test:e2e:watch": "npm run test:e2e --watch", isn't it strange 🤔


npm run test:e2e:watch    

> [email protected] test:e2e:watch
> npm run test:e2e -- --watch


> [email protected] test:e2e
> wp-scripts test-playwright --config test/e2e/playwright.config.ts --watch

error: unknown option '--watch'

up1512001
up1512001 approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@up1512001 up1512001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, @Mamaduka any suggestions. 🙇

ellatrix
ellatrix reviewed Dec 18, 2024
View reviewed changes
test/e2e/specs/editor/blocks/list.spec.js
<!-- wp:list-item -->
<li></li>
<!-- /wp:list-item -->
<!-- wp:list-item /-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not make any sense to me.

ellatrix
ellatrix requested changes Dec 18, 2024
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

It is missing a check while saving the list-item.

I don't understand this answer to "Why?"

This does not make sense to me at all. If the user adds a list item, it should be displayed.

@nani-samireddy
Copy link
Author

Closing this PR based on this comment

Thank you @ellatrix for confirming the expected behaviour of the list-item.

Thank you @up1512001 and @Mamaduka for taking time and reviewing this

@Mamaduka
Copy link
Member

Thanks for contributing, @nani-samireddy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellatrix ellatrix ellatrix requested changes

@up1512001 up1512001 up1512001 approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@fabiankaegy fabiankaegy Awaiting requested review from fabiankaegy fabiankaegy is a code owner

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

Assignees
No one assigned
Labels
[Block] List Affects the List Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty last List Element should not be displayed
4 participants
@nani-samireddy @up1512001 @Mamaduka @ellatrix